-
Notifications
You must be signed in to change notification settings - Fork 490
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
document multi-value headers as undefined #765
document multi-value headers as undefined #765
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hbagdi The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
apis/v1alpha2/httproute_types.go
Outdated
@@ -349,6 +349,9 @@ type HTTPHeaderMatch struct { | |||
// case-insensitivity of header names, "foo" and "Foo" are considered | |||
// equivalent. | |||
// | |||
// Matching behavior for headers with multiple values (like `Forwarded-for`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to get a PR together, but was hitting validation issues. This is what I wrote:
+++ b/apis/v1alpha2/httproute_types.go
@@ -343,12 +343,19 @@ type HTTPHeaderMatch struct {
// Name is the name of the HTTP Header to be matched. Name matching MUST be
// case insensitive. (See https://tools.ietf.org/html/rfc7230#section-3.2).
//
- // If multiple entries specify equivalent header names, only the first entry
- // with an equivalent name MUST be considered for a match. Subsequent
+ // If multiple entries specify equivalent header names, only the first
+ // entry with an equivalent name MUST be considered for a match. Subsequent
// entries with an equivalent header name MUST be ignored. Due to the
// case-insensitivity of header names, "foo" and "Foo" are considered
// equivalent.
//
+ // When a header is repeated in an HTTP request, it is
+ // implementation-specific behavior as to how this is represented.
+ // Generally, proxies should follow the guidance from the RFC:
+ // https://www.rfc-editor.org/rfc/rfc7230.html#section-3.2.2 regarding
+ // processing a repeated header values as a single comma (",")
+ // separated header, with special handling for "Set-Cookie".
+ //
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=256
Name string `json:"name"`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If multiple entries specify equivalent header names, only the first entry with an equivalent name MUST be considered for a match. Subsequent entries with an equivalent header name MUST be ignored.
I think that interpretation would run into the same vulnerability that Envoy ran into:
Attackers can set multiple values of a non-inline header, e.g. x-foo: bar and x-foo: baz. Affected Envoy components will only observe the first value, x-foo: bar, in matchers, but both x-foo: bar and x-foo: baz will be forwarded to upstreams. Upstreams may take both values into consideration, resulting in an inconsistency between Envoy’s request matching and the upstream view of the request.
An example of how this might be exploited is if an Envoy filter validates both x-request-credential and x-request-scope headers, checking whether the given credentials match the request scopes. Only the first scope header would be validated, but the upstream may interpret all provided scopes as being valid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference, their fix involved changing matching to use a comma separated list of header values, which seems like an appropriate requirement here: https://github.com/envoyproxy/envoy/blob/a12869fa9e9add4301a700978d5489e6a0cc0526/docs/root/version_history/v1.14.5.rst
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have two things, maybe want to clarify more:
- multiple HTTPHeaderMatch entries (which is what the first thing is talking about)
- multiple of the same header in the HTTP request -- which is somewhat malformed except for the comma ones; and the handling of those is up to the implementation. Good secure implementations merge header together using commas and drop extra headers for fields that don't support commas etc and Set-Cookie is an odd duck.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes a lot of sense, I was confusing the two. Agree with your suggested updates given that distinction.
@@ -349,6 +349,9 @@ type HTTPHeaderMatch struct { | |||
// case-insensitivity of header names, "foo" and "Foo" are considered | |||
// equivalent. | |||
// | |||
// Matching behavior for headers with multiple values (like `Forwarded-for`) | |||
// is not defined by this spec. | |||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Subsequent entries with an equivalent header name MUST be ignored.
So.. is someone going to change envoy to match like this or are we all just going to be non-compliant 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think our spec should actually more closely match the updated envoy behavior to avoid the vulnerability they ran into (referenced in comment above).
292a25e
to
23a73eb
Compare
Thanks! /lgtm |
What type of PR is this?
What this PR does / why we need it:
Clarifies that matching behavior for multi-value headers is undefined. This seems like a very rare use-case and specifying something that may cause conformance headaches doesn't seem to be worth it.
Which issue(s) this PR fixes:
Fixes #213
Does this PR introduce a user-facing change?: